Skip to content

Board identification: always check vid.1/pid.1 #463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Nov 4, 2019
Merged

Board identification: always check vid.1/pid.1 #463

merged 2 commits into from
Nov 4, 2019

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Oct 30, 2019

Even if vid.0/pid.0 is missing.

Fix #456

@intrigus

@cmaglie cmaglie self-assigned this Oct 30, 2019
@intrigus
Copy link

intrigus commented Oct 30, 2019 •
edited
Loading

Thank you for your swift reply!

How does the arduino ide do the identification?
(Since the arduino ide correctly identifies the board)
So it might be beneficial to copy the ide's code.

This PR will probably fix the problem but it feels a little bit "hacky" (I don't know go).
It is absolutely no problem for me to change the boards.txt file.
I.e. I technically don't "need" this PR, because I have full control over the boards.txt file.

Edit: I don't know go, so will this also work with starting wrongly at vid.2?

@cmaglie
Copy link
Member Author

cmaglie commented Oct 31, 2019

With this patch the cli will accept vid/pid starting from 0 or 1 but not from 2.
The Java IDE will accept any index, even starting from 2.

When I first looked at your issue it wasn't immediately clear the problem, I discovered it by comparing with another boards.txt, I think that the off-by-one may be a very common mistake and it's worth adding this corner case and relaxing a bit the specification.

Of course, if you can easily change your board.txt to start from 0, I would do it anyway :-)

matthijskooijman
matthijskooijman reviewed Nov 1, 2019
View reviewed changes
Copy link
Collaborator

@matthijskooijman matthijskooijman left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code change looks good to me, seems fine to support this common off-by-one.

One mostsly unrelated refactor suggestion, maybe change:

		again, found := checkSuffix(board.Properties, fmt.Sprintf(".%d", id))

To:

		present, matched := checkSuffix(board.Properties, fmt.Sprintf(".%d", id))

That is, change the variable names. It took me a while to see what these variables meant. In my suggestion, present means that the line in question is present in the config (but not necessarily matches the current id), and matched indicates it also matches the current id. That might make the code slightly more readable.

@cmaglie
Copy link
Member Author

cmaglie commented Nov 4, 2019

That might make the code slightly more readable.

Right I've renamed the variables, I'll merge as soon as the CI checks the PR

@cmaglie cmaglie force-pushed the cmaglie/fix-board-identify-with-id-1 branch from e25f08e to 6cdc384 Compare November 4, 2019 16:59
@cmaglie cmaglie merged commit 007b581 into master Nov 4, 2019
@cmaglie cmaglie deleted the cmaglie/fix-board-identify-with-id-1 branch November 4, 2019 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthijskooijman matthijskooijman matthijskooijman left review comments

Assignees

@cmaglie cmaglie

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Custom board is detected as "Unknown"
3 participants
@cmaglie @intrigus @matthijskooijman